-
Notifications
You must be signed in to change notification settings - Fork 411
Refactor ChannelManager handle functions into a Channel-closing macro #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor ChannelManager handle functions into a Channel-closing macro #147
Conversation
Original macro is from Antoine Riard <[email protected]>, the error type and additional mappings are from Matt Corallo <[email protected]>
Original version is from Antoine Riard <[email protected]>, the error type and some return type fixes are from Matt Corallo <[email protected]>
They are all just "its too early/late to get an announcement" errors so simply ignoring them and not sending an announce is fine
Because we've separated out channel closure from ErrorMessage returning we can return error messages in a few additional cases, like if the peer sent us a message for a channel they didn't own.
500582e
to
a71abac
Compare
err: msgs::HandleError, | ||
needs_channel_force_close: bool, | ||
} | ||
impl MsgHandleErrInternal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm having a builder is great to simplify errors filling but I think it's force us to specify a method for each case as a71abac shows it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe, though I also don't want to have ErrBuilder::new().msg().action().needs_close() every time we want to return an Err cause then we spend all our charachters on a line on error handling :/. Not sure if there's a great solution here, really, but I'm open to patches to play around with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, c-lightning doesn't have a concept of general handle error like us.
Still not delighted by this design, but don't see better for now so will go ahead with this for channel refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, well nothing stopping us from cleaning up things with macros or flattening errors in a future PR.
This is a rebase/rewrite/extension of #140 since it was missing some stuff and I wanted to get the jump on using it to move announcement_signatures handling into it so we can finally close up BOLT 7 handling.